stats: fix panic caused by outdated feedback #7128
Conversation
|
Do we need to cherry-pick this to the release-2.0? |
|
@shenli Yes, we need to. |
statistics/update.go
Outdated
| if err != nil { | ||
| return errors.Trace(err) | ||
| } | ||
| // Update the NDV of primary key column. |
There was a problem hiding this comment.
Why not check "hist != nil" here?
There was a problem hiding this comment.
We clear the outdated feedback in dumpStatsUpdateToKV, if check the hist here, we won't call that function, so these feedbacks will remain in the storage forever.
statistics/update.go
Outdated
| } | ||
| h.mu.Lock() | ||
| h.mu.ctx.GetSessionVars().BatchDelete = true | ||
| sql := fmt.Sprintf("delete from mysql.stats_feedback where table_id = %d and hist_id = %d and is_index = %d", tableID, hist.ID, isIndex) | ||
| sql := fmt.Sprintf("delete from mysql.stats_feedback where table_id = %d and hist_id = %d and is_index = %d", tableID, histID, isIndex) |
There was a problem hiding this comment.
Do we need to disable batch-delete after running the SQL?
statistics/update.go
Outdated
| hist.NDV = int64(hist.totalRowCount()) | ||
| col = nil | ||
| } | ||
| err = h.dumpStatsUpdateToKV(tableID, int(isIndex), histID, q, hist, cms) |
There was a problem hiding this comment.
It will be initialized when we meet a new group, as indicated here.
There was a problem hiding this comment.
If we group feedbacks in the first loop, then dump the feedbacks in the second loop, we can avoid the case that histID is -1.
|
LGTM |
|
/run-all-tests |
statistics/update.go
Outdated
|
|
||
| q := &QueryFeedback{} | ||
| for _, rows := range groupedRows { | ||
| tableID, histID, isIndex = rows[0].GetInt64(0), rows[0].GetInt64(1), rows[0].GetInt64(2) |
There was a problem hiding this comment.
If we extract the code in the loop in a function, then we can defer deleteOutdatedFeedback
statistics/update.go
Outdated
| hist = UpdateHistogram(hist, q) | ||
| err := h.SaveStatsToStorage(tableID, -1, int(isIndex), hist, cms, 0) | ||
| if err != nil { | ||
| metrics.UpdateStatsCounter.WithLabelValues(metrics.LblError).Inc() |
statistics/update.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (h *Handle) handleSingleHistogramUpdate(is infoschema.InfoSchema, rows []types.Row) (err error) { |
There was a problem hiding this comment.
please add some comment about:
- all the rows with the same "(tableId, histId, isIndex)" values from the feedback are gathered in
rows - this functions will update the Histogram and CMS using these feedbacks.
|
/run-all-tests |
What have you changed? (mandatory)
When feedback is outdated, for example, the table has been dropped, it should not cause panic. This PR fixes it by first checking that we do have the corresponding stats.
What is the type of the changes? (mandatory)
How has this PR been tested? (mandatory)
Unit test.
Does this PR affect documentation (docs/docs-cn) update? (mandatory)
No.
Does this PR affect tidb-ansible update? (mandatory)
No.
Does this PR need to be added to the release notes? (mandatory)
No.
PTAL @coocood @zz-jason @winoros